Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature] Allow removing elements from recently played #1937

Merged
merged 6 commits into from
Oct 27, 2022
Merged

[Feature] Allow removing elements from recently played #1937

merged 6 commits into from
Oct 27, 2022

Conversation

arielj
Copy link
Collaborator

@arielj arielj commented Oct 23, 2022 •
edited
Loading

This PR adds the feature to allow users to remove elements from the Recently Played area manually. Fixes #1319

It adds a few more things:

  • now we don't limit the list the store of the recently played list, we limit the list when displaying it, it allows to fill the list with more elements when removing one
  • refactored the code a bit moving code to a recent_games file
  • there was a bug where the order of the recently played games was not correct, it was ordered by the libraries order (epic games before gog, and also ordered within the same library). now the order is the order of the list

There's one issue that this PR doesn't fix (I'll add a report for it): the list of games in the tray icon menu doesn't get updated on the fly, it needs a restart of Heroic. Fixing that with the current code needs a lot of hacky coupled code between main, the context menu handler and the recently played list, if we implement an event system that's going to be a lot easier and cleaner to do.

Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@arielj arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label Oct 23, 2022
@arielj arielj requested review from a team, flavioislima, CommandMC, Nocccer and imLinguin and removed request for a team October 23, 2022 18:32
@flavioislima
Copy link
Member

There's one issue that this PR doesn't fix (I'll add a report for it): the list of games in the tray icon menu doesn't get updated on the fly, it needs a restart of Heroic. Fixing that with the current code needs a lot of hacky coupled code between main, the context menu handler and the recently played list, if we implement an event system that's going to be a lot easier and cleaner to do.

Maybe adding a listener to the recent_played store so we could generate the tray menu again could fix that.
We can reload the tray icon just like we are doing when we change the icon color.

@arielj
Copy link
Collaborator Author

arielj commented Oct 23, 2022

There's one issue that this PR doesn't fix (I'll add a report for it): the list of games in the tray icon menu doesn't get updated on the fly, it needs a restart of Heroic. Fixing that with the current code needs a lot of hacky coupled code between main, the context menu handler and the recently played list, if we implement an event system that's going to be a lot easier and cleaner to do.

Maybe adding a listener to the recent_played store so we could generate the tray menu again could fix that. We can reload the tray icon just like we are doing when we change the icon color.

You mean watching the .json file for changes? I think it would work yeah, I wonder how performant that is (not sure how listening for file changes work).

What I imagined was that idea of having an events system, so then we can emit the event recentGamesChanged in the backend and listen to it somewhere else in another module.

@flavioislima
Copy link
Member

@arielj about performance I think it is fine.
We can get feedback from the beta release, if it affects it then we try another solution.

@arielj arielj added pr:wip WIP, don't merge. and removed pr:ready-for-review Feature-complete, ready for the grind! :P labels Oct 26, 2022
@arielj
Copy link
Collaborator Author

arielj commented Oct 26, 2022 •
edited
Loading

@arielj about performance I think it is fine. We can get feedback from the beta release, if it affects it then we try another solution.

I just realized when coding this that we store the recently played games in the same file we store other things like favorites, the window size/position, log paths and more. I'll implement the watch for now, but I'll try to improve this after the beta is released

EDIT: for some reason, the watch function seems to not work to detect file changes in the ~/.config/heroic/store/config.json file, it never reports that it changes. Looks like it's a known thing that can happen (it looks like the electron store recreated the file instead of modifying the already existing one) nodejs/node#5039

@flavioislima flavioislima changed the title Allow removing elements from recently played [Feature] Allow removing elements from recently played Oct 27, 2022
flavioislima
flavioislima approved these changes Oct 27, 2022
View reviewed changes
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Tested here and it is working fine.

@flavioislima flavioislima merged commit 3af6e06 into beta Oct 27, 2022
@flavioislima flavioislima deleted the feature/remove-recent branch October 27, 2022 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flavioislima flavioislima flavioislima approved these changes

@CommandMC CommandMC Awaiting requested review from CommandMC CommandMC was automatically assigned from Heroic-Games-Launcher/development

@Nocccer Nocccer Awaiting requested review from Nocccer Nocccer was automatically assigned from Heroic-Games-Launcher/development

@imLinguin imLinguin Awaiting requested review from imLinguin imLinguin was automatically assigned from Heroic-Games-Launcher/development

Assignees
No one assigned
Labels
pr:wip WIP, don't merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arielj @flavioislima